-
Notifications
You must be signed in to change notification settings - Fork 431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Events 2.0
#1827
Events 2.0
#1827
Conversation
In the first iteration I implemented this solution, see the However I decided that this approach was introducing some non obvious coupling between the The implementation now is at least "encapsulated" in Of course the tradeoff is that if you reference a crate with an event but don't emit it, it still gets used in the metadata, as you demonstrate in #1842. However I would say that having an event in the metadata that could possibly be raised but is never raised is not such a big issue. It is even possible with any solution to have event which is emitted in a logically unreachable code branch it So on balance I have chosen the simpler solution. |
I was happy with this approach from the beginning #1243 (comment) =D I only highlighted that the problem is not solved(we discussed it during the previous iteration, and I thought is our final goal to solve it)=) |
* Usage of the crate includes defined there events * Inclusion of the event from `event_def_unused` is expected
Yes you were right all along :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmichi @SkymanOne The pull request is ready for a final review=) It looks good to me.
While we wait, here is the comparison of contract sizes. Used script from #1851, run on awk 'NR==FNR {a[$1]=$2; next} $1 in a {print "|" $1 "|" a[$1] "|" $2 "|" (($2 - a[$1]) / a[$1]) * 100 "|"}' master shared-events-redux
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Congrats on the PR. Just one last small nitpick
Rework the lint after use-ink#1827
…1837) * The initial structure of the linting crate and tests Needed for #1436 * Implement the lint that checks primitive numeric types in expanded code * The initial implementation of the `primitive_topic` lint * chore: Run clippy * fix(fmt) * fix: Lint suppressions The problem with suppressions is that `rustc` saves the state of the last visited node and checks its attributes to decide if the lint warning should be suppressed there (see: [LateContext::last_node_with_lint_attrs](https://github.com/rust-lang/rust/blob/c44324a4fe8e96f9d6473255df6c3a481caca76f/compiler/rustc_lint/src/context.rs#L1105)). This commit adds a call to the utility function from clippy that checks if the lint is suppressed for the field definition node, not for the last node (which is `Topics` impl). * chore: cleanup * chore: make warning messages more consistent * feat: support events 2.0 Rework the lint after #1827 is merged * chore: Fix comments; remove needless empty file * chore: Update `clippy_utils` version * fix: Cargo.toml --------- Co-authored-by: Georgiy Komarov <georgiy@parity.io>
Rework of event definitions:
#[ink::contract]
macro and changes the topic calculation, allowing events to be shared between contracts.Closes #809 and supersedes #1243.
New macros
Events can now be defined independently of contracts using the new
event
attribute macro:This event can now be imported and emitted by any contract e.g.
Derives
The
ink::event
attribute macro simply expands to underlyingderive
macros for the following traits:Event
(formerlyTopics
): captures the topics of the event when it is emitted at runtime.EventMetadata
: returns the metadata for the event definition.e.g.
expands to
Backwards compatibility
The legacy syntax of "inline" event definitions using the
#[ink(event)]
syntax will continue to work, no code changes in existing contracts are required to update to the new syntax.Topics changes
How topics are calculated has changed, so this is a breaking change for any client code which uses topics to filter events.
Signature topic
Currently, the signature topic of an event was calculated by
<contract_name>::<event_name>
e.g."Erc20::Transfer"
, which in turn was hashed if encoded len > 32 bytes.Now, we mimic the approach of Solidity events of hashing the name of the event and the concatenated field types e.g. given
Produces the signature of:
Transfer(Option<AccountId>, Option<AccountId>, Balance)
Which is then hashed with
blake2x256
to produce the signature topic. This is calculated by the macro at compile time, and can be accessed statically with<Transfer as Event>::SIGNATURE_TOPIC
. If the event is annotated with#[ink(anonymous)]
attribute then the value of this will beNone
and no signature topic is published.Field type name variation
In Rust, the same concrete type can be represented in different ways via type aliases and associated types e.g.
u32, MyAlias, T::AssocType
can all represent the same concrete type but with different names in the source code. This can result in two events with the same name and the same field types having different signature topics. This issue is discussed further with @xgreenx #1243 (comment).In the end I decided that it is an acceptable compromise that two binary compatible events would have different signature topics. It will be important to document this fact, where e.g. for block explorers which want to index on common events such as
Transfer
that there is a canonical definition of the event which must have matching field type names.Field topics
Previously field topics were calculated by concatenating the path to the field e.g.
Erc20::Transfer::from
with the value of the field, then hashing it (if > 32 bytes).However with this encoding topic values are often opaque, i.e. from a topic value on it's own there is no way to know the actual value of the field in the topic because it has been hashed. Even if it is < 32 bytes and not been hashed, the path of the field prefixed the value, so the value itself can not easily be separated.
Specifically it is a problem for topics with the
AccountId
type, which is often indexed. Indexers such as block explorers might want a view which shows all events affecting a givenAccountId
. With the current topics encoding, this would be difficult, requiring the metadata for all known events and having to precalculate hashes for topics with known account ids.With the new topics, the value of the event field itself (if <= 32 bytes) is the topic. In the case of 32 byte account ids this allows subscribing to a topic for a given account id and then it is guaranteed to receive all events which relate to that account.
It is still possible to filter those events which are from a specific event type, by combining with the filter with the signature topic. The topics data structure in a block is effectively a map of topics to event indices i.e.
HashSet<Hash, Vec<u32>>>
. So a filter would apply a union to the event indices of the combined topics.This is all similar to how topics/logs work on EVM chains.
Fields with the
Option
typeOption
fields are a special case. If a topic field value isNone
then it pushes the0x0000...
topic, the same as ifNone
was encoded and converted to[u8; 32]
.However, the difference is if there is a value present, instead of encoding the
Option<T>
it encodes the value ofT
and uses that as a topic. This is a special case for fields of typeT
andOption<T>
to have matching topic values. Otherwise a suffix of1u8
for the encodedSome
value would prevent the topics being unified. And in the case of theAccountId
example above it would result in unnecessary hashing because the length of the encoded value would now be 33 bytes.Metadata
When events were all defined within the
ink::contract
macro it was easy to gather the metadata for all possible events being raised by a contract.However with "shared" events now able to be defined and raised anywhere inside or outside of the
ink::contract
definition, the challenge was how to gather all those possible events together and put them in the metadata.In the end I have used https://github.com/dtolnay/linkme for link time concatenation of event metadata into the
EVENTS
global static. See https://github.com/paritytech/ink/blob/ca861ae7eab19625e3e212be329b185a6219521f/crates/metadata/src/lib.rs#L164See original discussion #1243 (comment)
Topics Validation
Max environment topics.
Previously there were compile time checks to ensure that any given event did not have more topics than the max allowed for the given environment. This prevents a possible runtime error at compile time.
However with the event codegen being separated from the contract itself, this compile time check became more difficult, primarily because of the limitation of not being able to assert trait constant values of type parameters (i.e. ::MAX_EVENT_TOPICS)`.
I was able to find a solution to that by using a
const
type parameter and some codegen, but I found the tradeoffs unsatisfactory. So I moved the validation to the metadata generation stage, where it became a simple check at metadata generation time to see whether the number of topics exceeded the environment specification.Signature topic collisions
The metadata generation step also ensures that in the scope of the contract, signature topics are unique. This is required (see below) to decide the type of the event to decode when dealing with contract events.
Consuming Events
Previously the
ink::contract
macro stitched all the#[ink(event)]
definitions into a top level generated enum. The way to decode any events originating from the contract would be to use the enum encoding of the first byte to determine which event variant to decode to.Now, the signature topics must be used to determine which event a contract has emitted. See use-ink/cargo-contract#1184.
Contract Sizes
The simplification of topics codegen should reduce the contract sizes.
With
ink-waterfall
currently not working, the automatic checking of contract size differences is not available. Here are some examples to demonstrate some size reductions:erc20
7.5K
->7.1K
= -5.3%erc1155
16.9K
->15.8K
= -6.5%Naturally reductions depend on how many events are being defined in each contract, so would be good to take that into account to.